-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Bug fix - do not display empty bars when line.width is zero #4056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
"type": "bar", | ||
"marker": { | ||
"line": { | ||
"width": [10, 0, 10, 0, 10] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this mock only has array marker.line.width
value. Could you add one trace that test scaler marker.line.width
?
Maybe you can leave this mock alone and simply add another mock using https://codepen.io/MojtabaSamimi/pen/bXbmba , so that we also test the relative-stacked case.
Thanks for taking this one @archmoj - I made two minor comments. |
move getLineWidth function to helpers and reuse it in hover also fixing 4057
@@ -64,3 +64,12 @@ exports.getValue = function(arrayOrScalar, index) { | |||
else if(index < arrayOrScalar.length) value = arrayOrScalar[index]; | |||
return value; | |||
}; | |||
|
|||
exports.getLineWidth = function(trace, di) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bars don't have line.width
, but still calling this getMarkerLineWidth
would be better.
for(var i = 0; i < imax; i++) cd[i][cdAttr] = isNumber ? +traceAttr[i] : traceAttr[i]; | ||
for(var i = 0; i < imax; i++) { | ||
var v = traceAttr[i]; | ||
cd[i][cdAttr] = hasFn ? fn(v) : v; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what's faster
var hasFn = typeof fn === 'function';
for(var i = 0; i < N; i++) {
arrayOut[i] = hasFn ? fn(arrayIn[i]) : arrayIn[i];
}
or
fn = typeof fn === 'function' ? fn : Lib.noop;
for(var i = 0; i < N; i++) {
arrayOut[i] = fn(arrayIn[i]);
}
No need to change anything, I'm just thinking out-loud.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting question. And I like the minimalist version with Lib.noop
!
That may depend on the browser.
Having two loops may also be fast.
var i = 0;
if (typeof fn === 'function') {
for(; i < N; ++i) arrayOut[i] = fn(arrayIn[i]);
} else {
for(; i < N; ++i) arrayOut[i] = arrayIn[i];
}
Nicely done 💃 Thanks for the fixes! |
Fixes #4047.
Empty
bar
andfunnel
rectangles should not be displayed when marker.line.width is set to zero.Codepen after
Also addressing #4057 for bars in this PR.
Codepen after
@plotly/plotly_js